fix(ibc-core): reject packets with no timeouts in send_packet_validate
#1205
Conversation
|
Looks like these two tests (without any timeout) are expected to pass. ibc-rs/ibc-testkit/tests/core/router.rs Lines 347 to 359 in 16666c2 Judging by the ibc-rs/ibc-apps/ics20-transfer/types/src/msgs/transfer.rs Lines 65 to 67 in 16666c2 |
rnbguy
changed the title
send_packet_validate checks for timeoutssend_packet_validate
| @@ -84,7 +84,7 @@ mod tests { | |||
|
|
|||
| let proof_height = 10; | |||
| let default_raw_packet = dummy_raw_packet(proof_height, 1000); | |||
| let raw_packet_no_timeout_or_timestamp = dummy_raw_packet(10, 0); | |||
| let raw_packet_no_timeout_of_timestamp = dummy_raw_packet(10, 0); | |||
There was a problem hiding this comment.
maybe?
| let raw_packet_no_timeout_of_timestamp = dummy_raw_packet(10, 0); | |
| let raw_packet_no_timeout_timestamp = dummy_raw_packet(10, 0); |
| if !packet.timeout_height_on_b.is_set() && !packet.timeout_timestamp_on_b.is_set() { | ||
| return Err(ContextError::PacketError(PacketError::MissingTimeout)); | ||
| } |
There was a problem hiding this comment.
shouldn't we do a similar check in recv, timeout and timeoutOnClose?
All the on_a vs on_b cause a lot of duplicate code, ideally this check should be done in one place for all messages that include a packet.
There was a problem hiding this comment.
Very good question! Here's my argument:
- OnRecvPacket: timeout doesn't matter - as this would generate packet_ack anyway.
- OnTimeout/OnTimeoutClose: we added
send_packetwith timeout height or timeout timestamp (invariant).
Also, I didn't find these checks in IBC spec and in ibc-go.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1205 +/- ##
==========================================
+ Coverage 64.44% 64.45% +0.01%
==========================================
Files 229 229
Lines 22051 22055 +4
==========================================
+ Hits 14210 14215 +5
+ Misses 7841 7840 -1 ☔ View full report in Codecov by Sentry. |
…te` (#1205) * correct name * add test for packet with no timeout * ensure packet timeout is set * rename var * fix test compile * packet without timeout should fail --------- Co-authored-by: Sean Chen <[email protected]>
Closes: #1198
Description
send_packet_validateshould reject packets that have neither timeout height nor timestamp.Reference to ICS04 spec.
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.